feat: new course outline header [FC-0114]#2735
Conversation
|
Thanks for the pull request, @navinkarkera! This repository is currently maintained by Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
7a8db8f to
780ae32
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2735 +/- ##
==========================================
+ Coverage 94.90% 94.91% +0.01%
==========================================
Files 1235 1238 +3
Lines 28280 28398 +118
Branches 6415 6475 +60
==========================================
+ Hits 26838 26955 +117
- Misses 1371 1372 +1
Partials 71 71 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
7e4e3e3 to
bb120b9
Compare
| async function load() { | ||
| try { | ||
| // @ts-ignore | ||
| // eslint-disable-next-line import/no-extraneous-dependencies, import/no-unresolved | ||
| const module = await import('@edx/frontend-plugin-notifications'); | ||
| const hookFn = module.useAppNotifications ?? module.default; | ||
| if (!cancelled) { | ||
| // `module.useAppNotifications` is itself a hook | ||
| setHook(() => hookFn); | ||
| } | ||
| } catch (err: any) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Failed to load notifications plugin:', err); | ||
| } | ||
| } |
There was a problem hiding this comment.
@edx/frontend-plugin-notifications being installed as a plugin forces us to dynamically import it and stub it in webpack in case it is not installed.
There was a problem hiding this comment.
Is this the first time we have had this pattern?
Maybe we should have a more generic version of the useDynamicHookShim in generic if we need that every time we use a hook from a plugin.
There was a problem hiding this comment.
I think this is the first time, we could create a generic one if required but I don't think it is worth it right now. I am not even sure if this is the best way 😅
It could be much better to create the notification icon component in @edx/frontend-plugin-notifications repo and create a plugin slot here and include it same as the notification tray.
rpenido
left a comment
There was a problem hiding this comment.
Awesome work here @navinkarkera!
I tried checking the notification status bar, but it didn't appear. This is what I have on the /api/notifications/count response:
{"show_notifications_tray":false,"count":0,"count_by_app_name":{"discussion":0,"updates":0,"grading":0},"notification_expiry_days":60}
Do I need any additional setup to make it work (besides installing the plugin and running the migrations)?
I'm having this error, but I'm not sure if it is related:
cms-1 | edx_event_bus_redis.internal.producer.EventProductionException: Error -2 connecting to edx.devstack.redis:6379. Name or service not known.
| /* istanbul ignore file */ | ||
| import React from 'react'; | ||
|
|
||
| export interface HooKType { |
There was a problem hiding this comment.
typo
| export interface HooKType { | |
| export interface HookType { |
|
|
||
| // Load the hook module asynchronously | ||
| export function useDynamicHookShim() { | ||
| const [hook, setHook] = React.useState<(() => HooKType) | null>(null); |
There was a problem hiding this comment.
typo
| const [hook, setHook] = React.useState<(() => HooKType) | null>(null); | |
| const [hook, setHook] = React.useState<(() => HookType) | null>(null); |
|
|
||
| async function load() { | ||
| try { | ||
| // @ts-ignore |
There was a problem hiding this comment.
I'm not getting typescript errors from this
| // @ts-ignore |
There was a problem hiding this comment.
If @edx/frontend-plugin-notifications is not installed, typescript complains.
| import messages from './messages'; | ||
| import initializeStore from '../../store'; |
There was a problem hiding this comment.
Nit
| import messages from './messages'; | |
| import initializeStore from '../../store'; | |
| import initializeStore from '@src/store'; | |
| import messages from './messages'; |
There are also some paths in this file from mock that can be updated:
i.e
jest.mock('../../generic/data/api', () => ({
to
jest.mock('@src/generic/data/api', () => ({
| async function load() { | ||
| try { | ||
| // @ts-ignore | ||
| // eslint-disable-next-line import/no-extraneous-dependencies, import/no-unresolved | ||
| const module = await import('@edx/frontend-plugin-notifications'); | ||
| const hookFn = module.useAppNotifications ?? module.default; | ||
| if (!cancelled) { | ||
| // `module.useAppNotifications` is itself a hook | ||
| setHook(() => hookFn); | ||
| } | ||
| } catch (err: any) { | ||
| // eslint-disable-next-line no-console | ||
| console.error('Failed to load notifications plugin:', err); | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this the first time we have had this pattern?
Maybe we should have a more generic version of the useDynamicHookShim in generic if we need that every time we use a hook from a plugin.
| "paths": { | ||
| "@src/*": ["./*"] | ||
| "@src/*": ["./*"], | ||
| "CourseAuthoring/*": ["./*"] |
There was a problem hiding this comment.
Do you mind sharing the reason for this new alias? Are they used inside the plugins?
There was a problem hiding this comment.
It is used in a lot of places, specially in plugin-slot files like src/plugin-slots/CourseUnitHeaderActionsSlot/index.jsx. Also mentioned in https://github.com/open-craft/frontend-app-authoring/blob/bb120b9c739ab042bd9b5e4ae605c27209561719/jest.config.js#L16
There was a problem hiding this comment.
This CourseAuthoring/ alias is meant [only] for use in plugins, yep.
@rpenido I added this line to the instructions, let me know if it still doesn't work. |
bb120b9 to
ed0e2e9
Compare
rpenido
left a comment
There was a problem hiding this comment.
LGTM 👍
Thank you for your work, @navinkarkera!
- I tested this using the instructions from the PR
- I read through the code
- I checked for accessibility issues
- Includes documentation
ChrisChV
left a comment
There was a problem hiding this comment.
The code looks good, thanks! 👍
Description
Adds new header and subheader to course outline. Converts existing js code to ts.
Useful information to include:
Screenshots:
Supporting information
Private-ref: https://tasks.opencraft.com/browse/FAL-4290Testing instructions
ENABLE_COURSE_OUTLINE_NEW_DESIGNflag in env files. Make sure it is set totrue.Activebadge is displayed in the header.Upcomingbadge is displayedArchivedbadge is displayedtutor dev launch -I --skip-buildto initialize the plugin and allow it to create flags etc.env.config.jsxfile in authoring root folder.npm install @edx/frontend-plugin-notifications@^2.0.3in authoring foldertutor dev restart authoring.Content -> Course updatespage to create some notifications.Other information
Make sure to update tutor to latest version.
Best Practices Checklist
We're trying to move away from some deprecated patterns in this codebase. Please
check if your PR meets these recommendations before asking for a review:
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'